Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve testing framework #286

Merged
merged 28 commits into from
Oct 16, 2023
Merged

Improve testing framework #286

merged 28 commits into from
Oct 16, 2023

Conversation

rinon
Copy link
Collaborator

@rinon rinon commented Sep 21, 2023

No description provided.

@rinon rinon changed the title Convert two_keys_minimal to use criterion Improve testing framework Sep 22, 2023
@rinon rinon marked this pull request as ready for review September 22, 2023 23:30
@rinon
Copy link
Collaborator Author

rinon commented Sep 22, 2023

@sim-immunant here's my start to cleaning up the testing framework. I don't think we're getting much out of lit now that we can't do in other ways, perhaps with custom cmake tests that run FileCheck directly?

More context is in #154

fw-immunant
fw-immunant approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good--criterion test entrypoints are way better than having to parse arguments to decide which test case to run.

@fw-immunant
Copy link
Contributor

I would absolutely like to get rid of lit because it has way more complexity than I think we benefit from. The other major thing I think we're doing wrong in testing is testing some properties at every site where they're interacted with instead of once or in one all-encompassing pass. For example, we have // LINKARGS: --wrap=foo comments above every wrapped function to test if the wrappers we generate are correct. I don't think this is important coverage, and it would be better to separate unit-testing the rewriter (which is what the linkargs validation does) from our integration testing for IA2 as a whole.

rinon
rinon commented Sep 27, 2023
View reviewed changes
rewriter/tests/mmap_loop/main.c Outdated
int res = munmap(lib_buf_mmap, 4096);
cr_log_info(res ?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistically I prefer putting separate calls under and if branch, especially since we already have one.

rewriter/tests/ro_sharing/main.c Outdated
Test(ro_sharing, plugin, .exit_code = 255) {
cr_log_info("%s", get_plugin_str());
cr_log_info("0x%x", *get_plugin_uint(false));
cr_log_info("0x%x", *get_plugin_uint(true));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we put the CHECK_VIOLATION around the get_plugin_uint deref?

rewriter/tests/ro_sharing/plugin.c Outdated

// Check that we can't read a pointer to data passed from main
// TODO can we change this LOG to cr_log_info?
CHECK_VIOLATION(LOG("0x%x", *secret));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, change this to a cr_log and put the CHECK_VIOLATION around the *secret if we can?

ayrtonm
ayrtonm reviewed Sep 29, 2023
View reviewed changes
rewriter/tests/global_fn_ptr/operations.c
// LINKARGS: --wrap=call_operation
uint32_t call_operation(size_t i) {
// TODO: Add a way to share strings between compartments
//printf("%s\n", operations[i].desc.data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we can remove the comment on the printf and the TODO We previously treated the .rodata sections (where string literals are placed) as secret, but we have since moved on to only treating writable sections that way.

rinon
rinon commented Oct 5, 2023
View reviewed changes
rewriter/tests/sighandler/main.c Show resolved Hide resolved
rewriter/tests/simple1/main.c Show resolved Hide resolved
@ayrtonm ayrtonm mentioned this pull request Oct 10, 2023
Closed
rinon
rinon commented Oct 16, 2023
View reviewed changes
Copy link
Collaborator Author

@rinon rinon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I can't hit the approve button because I own the PR still. That's fine, just address the existing comments, resolve what's taken care of, and if it all looks good land it.

rewriter/tests/heap_two_keys/main.c Outdated Show resolved Hide resolved
rewriter/tests/macro_attr/functions.c Show resolved Hide resolved
@sim-immunant sim-immunant merged commit b9a1d68 into main Oct 16, 2023
33 checks passed
@sim-immunant sim-immunant deleted the sjc/improve_testing branch October 16, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sim-immunant sim-immunant sim-immunant left review comments

@ayrtonm ayrtonm ayrtonm approved these changes

@fw-immunant fw-immunant fw-immunant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rinon @fw-immunant @ayrtonm @sim-immunant